Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ignore_idx to retrieval metrics #676

Merged
merged 22 commits into from
Dec 13, 2021
Merged

Add ignore_idx to retrieval metrics #676

merged 22 commits into from
Dec 13, 2021

Conversation

lucadiliello
Copy link
Contributor

What does this PR do?

Fixes #672

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Dec 10, 2021 •
edited
Loading

Codecov Report

Merging #676 (a7d0295) into master (d071eb2) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #676   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         166    166           
  Lines        6370   6377    +7     
=====================================
+ Hits         6065   6070    +5     
- Misses        305    307    +2

@Borda Borda changed the title implemented feature #672 Add ignore_idx to retrieval metrics Dec 10, 2021
@Borda Borda added API / design enhancement New feature or request labels Dec 10, 2021
@Borda Borda added this to the v0.7 milestone Dec 10, 2021
@mergify mergify bot added ready and removed has conflicts labels Dec 10, 2021
@lucadiliello
Copy link
Contributor Author

Seems to me that we should reduce the number of tests, or maybe reduce their complexity. Do you know whether I can check how much time was taken just by the retrieval package?

@Borda
Copy link
Member

Borda commented Dec 10, 2021 •
edited
Loading

Seems to me that we should reduce the number of tests, or maybe reduce their complexity. Do you know whether I can check how much time was taken just by the retrieval package?

yes, it becomes a heavy bit I guess the most is coming from Image and Text packages...
you can run pytest with --durations=0 which ends with times over all/each test and you can aggregate it... just thinking about it, it would be a great analysis for us to identify what shall we improve 🐰

on the other note, I am not very sure if we shall lower test coverage, rather we can think about running some job concurrency and reducing configuration with a limited number of agents (the MacOs is limited and also the longest)
cc: @SkafteNicki @justusschock

@Borda Borda mentioned this pull request Dec 10, 2021
Closed
SkafteNicki
SkafteNicki reviewed Dec 13, 2021
View reviewed changes
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each metric have a Raises section in their docstring, where value error for ignore_index should be added.

torchmetrics/retrieval/retrieval_hit_rate.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_metric.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_ndcg.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_precision.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_recall.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_precision.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_ndcg.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_hit_rate.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_fallout.py Outdated Show resolved Hide resolved
torchmetrics/retrieval/retrieval_fallout.py Outdated Show resolved Hide resolved
lucadiliello and others added 10 commits December 13, 2021 15:21
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_ndcg.py
9868995 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_precision.py
3cfa5a0 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_recall.py
bfba4fe 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_recall.py
00e777d 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/utilities/checks.py
c05cd79 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_precision.py
6663552 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_ndcg.py
ec6c510 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_hit_rate.py
192ad7e 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_fallout.py
c14fb55 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@lucadiliello @SkafteNicki
Update torchmetrics/retrieval/retrieval_fallout.py
2c38938 
Co-authored-by: Nicki Skafte Detlefsen <[email protected]>
@mergify mergify bot removed the ready label Dec 13, 2021
@Borda Borda requested a review from SkafteNicki December 13, 2021 14:43
@mergify mergify bot added the ready label Dec 13, 2021
@Borda Borda enabled auto-merge (squash) December 13, 2021 16:36
@Borda Borda merged commit 01f88fe into Lightning-AI:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@Borda Borda Borda approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton

Assignees
No one assigned
Labels
API / design enhancement New feature or request ready
Projects
None yet
Milestone
v0.7
Development

Successfully merging this pull request may close these issues.

Add ignore_idx to retrieval metrics
3 participants
@lucadiliello @Borda @SkafteNicki